Add Wallet Integration Tests and Fix Descriptor Persistence Issues by moisesPompilio · Pull Request #821 · getfloresta/Floresta

moisesPompilio · 2026-02-02T18:59:50Z

Description and Notes

Adds comprehensive integration tests for wallet functionality and fixes several issues related to descriptor persistence and handling.

Key fixes:

Duplicate descriptor prevention: Added centralized validation to prevent the same descriptor from being stored multiple times in the database. The push_descriptor method now checks if a descriptor already exists before adding it.
Improved xpub parsing and descriptor generation:
- Xpub parsing now rejects xprivs and multisig-related xpubs
- Fixed descriptor generation to dynamically create wpkh, pkh, or sh-wpkh descriptors based on the provided xpub (previously only wpkh was generated)
- Added network validation to ensure xpubs match the node's network

key improvements:

Centralized address derivation: Address derivation logic has been moved to floresta-watch-only. When a descriptor is added, addresses are automatically derived and cached, simplifying wallet initialization and management.
Consolidated descriptor parsing: Moved parse_descriptors, parse_xpubs, and slip132 logic to the floresta-watch-only module, removing scattered code from floresta-node and floresta-common. Removed the miniscript dependency from floresta-common.
Documentation clarity: Updated wallet_descriptor option documentation to accurately describe descriptor behavior (previously incorrectly referred to xpubs).
Typos configuration: Added _typos.toml to prevent the typos checker from analyzing strings between 32-150 characters (hashes, keys, addresses, descriptors).

New integration tests:

wallet_conf.py: Tests wallet loading via config.toml, verifying correct descriptor/xpub loading and rejection of xprivs and private key descriptors
wallet_flag.py: Tests wallet loading via command-line flags with the same validations
RPC method tests for loaddescriptor (including duplicate detection and private key rejection) and listdescriptors
Constants centralization: Created constants.py to centralize all constants used across integration tests, improving maintainability.

How to verify the changes you have done?

Run the integration tests using the standard command:
```
./tests/run.sh
```
Verify that all new wallet tests (wallet_conf.py, wallet_flag.py, loaddescriptor, listdescriptors) pass successfully.
Test that duplicate descriptors are properly rejected when using loaddescriptor RPC method.
Confirm that xprivs and descriptors with private keys are rejected during wallet initialization.
Verify that xpubs are correctly validated against the node's network and generate appropriate descriptor types (wpkh, pkh, sh-wpkh).

jaoleal · 2026-02-02T22:50:55Z

The first commit seems unrelated

moisesPompilio · 2026-02-02T22:59:28Z

The first commit seems unrelated

The first commit introduces constants.py which centralizes the constants used in the code that are later used in the wallet tests, to store the xpubs and descriptors.

jaoleal

Concept ACK.

Heres some initial review

crates/floresta-node/src/error.rs

crates/floresta-node/src/florestad.rs

crates/floresta-watch-only/src/lib.rs

jaoleal · 2026-02-02T23:26:47Z

tests/floresta-cli/listdescriptors.py

+        self.run_node(self.florestad)
+
+        self.log("Checking initial descriptors...")
+        descriptors = self.florestad.rpc.list_descriptors()


One thing to add, both listdescriptors.py and loaddescriptor.oy is fine but they are dependent on each other, if one fails they will both fail...

that will be a problem if breaking one raises a false alarm about the other.

In integration tests it's common to have some interdependencies, such as between wallet_conf and wallet_flag. They also depend on listdescriptors, because that's the single method we use to check the descriptors Floresta has stored.

Yeah, fine to depend on listdescriptors, but listdescriptors test dont need to depend on loaddescriptor you can instantiate and insert a descriptor from the Config.toml. It can help for this case.

But in this case, if there is a problem with listdescriptors, the wallet_conf and wallet_flag tests will also fail, because they also depend on that RPC.

crates/floresta-node/src/florestad.rs

crates/floresta-watch-only/src/descriptor/mod.rs

jaoleal · 2026-02-02T23:30:49Z

crates/floresta-watch-only/src/descriptor/mod.rs

+/// Parses each descriptor, validates it, and derives the specified number of addresses
+/// starting from the given index.
+pub fn derive_addresses_from_list_descriptors(
+    descriptors: &[String],


Same thing from: https://github.com/getfloresta/Floresta/pull/821/changes/146e2ffb3060c633a40904a84de29bbe39c56363..27e55ad8c35c6d9d3aadb9dd3d20560b6d397a42#r2756460303

The descriptor is already a String, so using a &[&str] will make us waste resources converting it to &str. It's better to use it as a String directly; this method is used inside the wallet internals which already obtain the descriptor as a String.

Theres no cost casting to a &str. but i understand thats how our codebase is written right now, fine.

IIUC theres no cost at all to cast Strings to &strs, but theres some when casting &str to String, basically the same relation between [B] and Vec<B>... Being picky with this may lead us to less runtime allocs.

The ideal is to declare functions that only read and compare strings with a generic S where S: AsRef<str>

The problem here is that we can have multiple database implementations; so if we pass only pointers (&) to objects retrieved from the database it will be wrong, because the objects live shorter than the pointers. It's better to pass the object in this case, for example we pass a Vec<String> for descriptors, because if we passed Vec<&str> the &str would need to live longer than the Vec<String> that only exists inside the descriptor-retrieval function.

That's why when we receive a Vec<String> and it needs to be used by another function, that function takes a &[String] to avoid allocating memory by creating a new array of strings.

Davidson-Souza · 2026-02-03T19:09:45Z

Needs rebase

moisesPompilio · 2026-02-04T19:34:07Z

c2bff7d Performed a rebase and added some review requests.

Davidson-Souza · 2026-02-04T22:36:55Z

In ae433c1 there's still a REGTEST_RPC_SERVER constant in rpc/{floresta, utreexod}.py. I think it should also be moved?

moisesPompilio · 2026-02-05T15:12:24Z

In ae433c1 there's still a REGTEST_RPC_SERVER constant in rpc/{floresta, utreexod}.py. I think it should also be moved?

I didn't add them because they will be removed in this PR #733 .

moisesPompilio · 2026-02-06T21:52:51Z

5ce80a0 I did the rebase, adjusted some parts of the code that were using String where they could use &str, and removed the descriptors-no-std feature from CI (introduced by PR #825).

moisesPompilio · 2026-02-09T19:40:35Z

7f9b79b I did the rebase.

crates/floresta-watch-only/src/lib.rs

crates/floresta-node/src/florestad.rs

crates/floresta-watch-only/src/lib.rs

jaoleal · 2026-02-09T21:56:15Z

crates/floresta-node/src/wallet_input.rs

-fn parse_xpubs(xpubs: &[String]) -> Result<Vec<Descriptor<DescriptorPublicKey>>, FlorestadError> {
+fn parse_xpubs(
+    xpubs: &[String],
+    network: Network,


I think you dont need to take ownership to read enums

I couldn't understand the comment here

You can read the network enum by reference

Copy types should never be passed by reference

crates/floresta-node/src/wallet_input.rs

tests/test_framework/rpc/floresta.py

crates/floresta-common/src/lib.rs

crates/floresta-node/src/florestad.rs

crates/floresta-watch-only/src/descriptor/mod.rs

Created a `constants.py` file to centralize all constants used in the test framework. This change improves maintainability by providing a single location for managing constants and makes it easier to understand their purpose and usage across the tests.

Updated the documentation for the `wallet_descriptor` option to explicitly describe its behavior with descriptors. The previous text incorrectly referred to xpubs, which could cause confusion.

Previously, it was possible to save the same descriptor multiple times, leading to redundancy and potential inconsistencies. Adds a verification step in the `push_descriptor` method to ensure that a descriptor is only added if it does not already exist.

- Updated xpub parsing to reject xpriv and xpubs related to multisig, ensuring only standard public keys are accepted for descriptor generation. - Fixed an issue in descriptor generation by xpub where only `wpkh` descriptors were being created. Descriptors are now generated dynamically as `wpkh`, `pkh`, or `sh-wpkh`, depending on the xpub provided. - Added a network validation step to ensure the xpub matches the network the node is running on. - Added \_typos\.toml configuration to prevent https://github.com/crate-ci/typos from analyzing words with lengths between 32 and 150 characters, as these correspond to hashes, public keys, private keys, XPUBs, descriptors, and Bitcoin addresses.

- Added `wallet_conf.py` to test wallet loading via `config.toml`. This test verifies: - Descriptors and XPUBs are correctly loaded from the configuration file. - XPRIVs and descriptors with private keys are rejected. - Added `wallet_flag.py` to test wallet loading via command-line flags. This test performs the same checks as `wallet_conf.py`, but uses flags to pass wallet information during node initialization.

- Added tests to verify that the `loaddescriptor` RPC method can successfully load a descriptor. - Ensured that duplicate descriptors cannot be loaded. - Verified that descriptors containing private keys are rejected.

…thod - Added tests to verify that the `listdescriptors` RPC method correctly displays the descriptors loaded in the node. - Ensured that the method returns all loaded descriptors in the expected format.

moisesPompilio · 2026-02-15T19:12:21Z

21eca0d I've applied the review suggestions from @jaoleal

…-only` module Improved code organization by consolidating descriptor handling in a single module - Moved `parse_descriptors`, `parse_xpubs`, and `slip132` to the `floresta-watch-only` module. - Centralized descriptor and XPUB parsing logic, previously scattered across `floresta-node` and `floresta-common`. - Removed descriptor-related features and the `miniscript` dependency from `floresta-common`.

… and refactor wallet handling - Centralized address derivation in `floresta-watch-only`: - Address derivation is now automatically performed when pushing a descriptor. - Added `push_xpub` method to convert `XPUBs` into `descriptors`, push them, and derive addresses for the wallet. - Removed `walletInput` file from floresta-node: - This file was no longer necessary as its functionality (`XPUB`, `descriptor`, and address derivation) is now handled automatically by `floresta-watch-only`.

moisesPompilio self-assigned this Feb 2, 2026

jaoleal reviewed Feb 2, 2026

View reviewed changes

moisesPompilio force-pushed the add-test-wallet branch from 27e55ad to c2bff7d Compare February 4, 2026 19:32

moisesPompilio mentioned this pull request Feb 4, 2026

ci: add no-std job to check floresta-common #825

Merged

moisesPompilio force-pushed the add-test-wallet branch from c2bff7d to 5ce80a0 Compare February 6, 2026 21:47

jaoleal self-requested a review February 9, 2026 10:07

moisesPompilio force-pushed the add-test-wallet branch 2 times, most recently from 7b9921f to 7f9b79b Compare February 9, 2026 19:31

jaoleal reviewed Feb 10, 2026

View reviewed changes

moisesPompilio added 5 commits February 15, 2026 14:33

docs(cli): clarify documentation for wallet descriptor option

1755dbe

Updated the documentation for the `wallet_descriptor` option to explicitly describe its behavior with descriptors. The previous text incorrectly referred to xpubs, which could cause confusion.

moisesPompilio force-pushed the add-test-wallet branch from 7f9b79b to 4e34539 Compare February 15, 2026 18:56

moisesPompilio force-pushed the add-test-wallet branch from 4e34539 to e18de15 Compare February 15, 2026 19:02

moisesPompilio added 2 commits February 15, 2026 16:06

test(integration): add integration tests for listdescriptors RPC me…

afaf734

…thod - Added tests to verify that the `listdescriptors` RPC method correctly displays the descriptors loaded in the node. - Ensured that the method returns all loaded descriptors in the expected format.

moisesPompilio force-pushed the add-test-wallet branch from e18de15 to 1c97cb5 Compare February 15, 2026 19:06

moisesPompilio added 2 commits February 15, 2026 17:04

moisesPompilio force-pushed the add-test-wallet branch from 1c97cb5 to 21eca0d Compare February 15, 2026 20:04

Davidson-Souza mentioned this pull request Feb 17, 2026

0.9.0 priorities #850

Open

JoseSK999 added this to the v0.9.0 milestone Feb 18, 2026

Conversation

moisesPompilio commented Feb 2, 2026

Description and Notes

How to verify the changes you have done?

Uh oh!

jaoleal commented Feb 2, 2026

Uh oh!

moisesPompilio commented Feb 2, 2026

Uh oh!

jaoleal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Davidson-Souza commented Feb 3, 2026

Uh oh!

moisesPompilio commented Feb 4, 2026

Uh oh!

Davidson-Souza commented Feb 4, 2026

Uh oh!

moisesPompilio commented Feb 5, 2026

Uh oh!

moisesPompilio commented Feb 6, 2026

Uh oh!

moisesPompilio commented Feb 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moisesPompilio commented Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

moisesPompilio commented Feb 15, 2026 •

edited

Loading